Skip to content

treewide: remove refs/tags/ from github release meta.changelog#353000

Merged
Aleksanaa merged 5 commits intoNixOS:masterfrom
pbsds:fix-changelog-release-src-refs-1730497909
Nov 6, 2024
Merged

treewide: remove refs/tags/ from github release meta.changelog#353000
Aleksanaa merged 5 commits intoNixOS:masterfrom
pbsds:fix-changelog-release-src-refs-1730497909

Conversation

@pbsds
Copy link
Member

@pbsds pbsds commented Nov 1, 2024

I saw #346488 was a thing, so i figured a redo of #338301 was warranted. Part of #346453

diff of jq <packages.json 'to_entries[]|"\(.key) \(.value.meta.changelog)"' -r:

https://gist.github.com/pbsds/02ee3d771ab31e15c3c67db9c79c4e57

Made using the script in #338301 without modifications

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@pbsds pbsds changed the title forgejo: remove refs/tags/ from github release meta.changelog treewide: remove refs/tags/ from github release meta.changelog Nov 1, 2024
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Nov 1, 2024
@pbsds pbsds changed the title treewide: remove refs/tags/ from github release meta.changelog treewide: remove refs/tags/ from github release meta.changelog Nov 1, 2024
@pbsds pbsds marked this pull request as ready for review November 1, 2024 22:02
@SuperSandro2000
Copy link
Member

This change is kinda backwards. We should take version and optionally add the v prefix.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 1, 2024
Copy link
Contributor

@adamcstephens adamcstephens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Sandro that we should use version if these links are broken.

@pbsds
Copy link
Member Author

pbsds commented Nov 1, 2024

semantically the github release urls are derived from the git tag, which don't necessarily map cleanly from the version. In #338301 the idea of a tag input and output for fetchFromGitHub was brought up which I fully support, but in the meanwhile I believe this is the easiest treewide to consistently enforce.

@adamcstephens
Copy link
Contributor

adamcstephens commented Nov 2, 2024

I don't really understand. How is what you propose any simpler, clearer, or more reliable than using the version like so:

changelog = "https://codeberg.org/forgejo/forgejo/releases/tag/v${version}";

In my opinion your version is more complex, and needlessly spends compute cycles modifying strings when we already have said string in version. This is all so you don't have to put the v in front of version in the URL?

If an upstream is so far off from our versioning that you can't just re-use version, then treat that as the odd-ball that it is. There's no reason to apply premature optimization and complexity everywhere else because of edge cases.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Nov 2, 2024
@pbsds pbsds force-pushed the fix-changelog-release-src-refs-1730497909 branch from ff879c7 to 5792376 Compare November 2, 2024 17:25
@pbsds
Copy link
Member Author

pbsds commented Nov 2, 2024

Okay, have it your way. Note now that the review requires you to view the whole file. I assume someone will in the future come along and revert the change back to src.rev not knowing better.

I left forgejo with lib.removePrefix since rev is provided as an input to callPackage

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 2, 2024
@emilylange
Copy link
Member

I left forgejo with lib.removePrefix since rev is provided as an input to callPackage

version is used and required in multiple places in this internal abstraction (generic.nix) of Forgejo.
Please change it to v${version}, just like you did with all the other packages.

Thanks :)

@pbsds pbsds force-pushed the fix-changelog-release-src-refs-1730497909 branch from 5792376 to 2ec380a Compare November 5, 2024 11:50
@pbsds
Copy link
Member Author

pbsds commented Nov 5, 2024

Thanks for pointing that out 👍

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Nov 6, 2024
@Aleksanaa Aleksanaa merged commit f96eb6a into NixOS:master Nov 6, 2024
@philiptaron philiptaron mentioned this pull request Dec 5, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants